Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[output] PagerDuty incident requires "From:" header #493

Merged
merged 3 commits into from
Nov 20, 2017

Conversation

javuto
Copy link
Contributor

@javuto javuto commented Nov 20, 2017

to: @ryandeivert @jacknagz
cc: @airbnb/streamalert-maintainers
size: small

Background

New output to use PagerDuty Incidents was added here. The REST API requires the header From: validpagerdutyuser@email.com to create a new incident.

Changes

  • The header From: is added to authentication, and the user will be added when creating new output.
  • Modified method _check_exists to optionally return just a boolean.
  • All tests have been modified to accommodate this change.

Testing

$ rm .coverage && ./tests/scripts/unit_tests.sh
...
TOTAL                                                2516     76    97%
----------------------------------------------------------------------
Ran 462 tests in 7.015s

OK

$ ./tests/scripts/pylint.sh

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

$ python manage.py lambda test --processor rule all
...
StreamAlertCLI [INFO]: (59/59) Successful Tests
StreamAlertCLI [INFO]: (93/93) Alert Tests Passed
StreamAlertCLI [INFO]: Completed

@javuto javuto added this to the 1.6.0 milestone Nov 20, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.912% when pulling 3d3102b on javier-streamalert-pagerduty-incidents-bugs into 04ac9c1 on master.

@@ -291,21 +296,23 @@ def _check_exists_get_id(self, filter_str, url, target_key):
return False

# If there are results, get the first occurence from the list
return response[target_key][0]['id'] if target_key in response else False
if get_id:
Copy link
Contributor

@ryandeivert ryandeivert Nov 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do the inverse please

if not get_id:
    return True

# If there are results, get the first occurance from the list
return response[target_key][0]['id'] if target_key in response else False

Copy link
Contributor

@ryandeivert ryandeivert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @javuto good job catching this. some initial comments for ya

"""
item_id = self._check_exists_get_id(item_str, item_url, item_key)
item_url = self._get_endpoint(self._base_url, item_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good change, putting this in one place!

'type': item_type
}
if get_id:
return {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put this dict on one line:

return {id': item_id, 'type': item_type}

return self._log_status(False)

# Add From to the headers after verifying
self._headers['From'] = user_email
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this header value really title case (From)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird 🤣


def _user_verify(self, user):
def _user_verify(self, user, get_id=True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the only time this function is called, the default value for get_id is overridden with False... am I missing something? When would get_id ever be True here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In _incident_assignment it gets called and the default True value is used

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.918% when pulling 873d748 on javier-streamalert-pagerduty-incidents-bugs into 04ac9c1 on master.

@javuto javuto force-pushed the javier-streamalert-pagerduty-incidents-bugs branch from 873d748 to 1001bab Compare November 20, 2017 21:44
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.918% when pulling 1001bab on javier-streamalert-pagerduty-incidents-bugs into 0e45f5a on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 95.92% when pulling 0043af9 on javier-streamalert-pagerduty-incidents-bugs into 0e45f5a on master.

Copy link
Contributor

@ryandeivert ryandeivert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢 🐑

@javuto javuto force-pushed the javier-streamalert-pagerduty-incidents-bugs branch from 0043af9 to 6406c94 Compare November 20, 2017 22:04
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6406c94 on javier-streamalert-pagerduty-incidents-bugs into ** on master**.

@javuto javuto merged commit 1f5cb69 into master Nov 20, 2017
@javuto javuto deleted the javier-streamalert-pagerduty-incidents-bugs branch November 20, 2017 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants